Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix built-in HttpClient metrics documentation #39223

Merged
merged 2 commits into from
Feb 2, 2024
Merged

Conversation

xakep139
Copy link
Contributor

@xakep139 xakep139 commented Jan 23, 2024

Summary

Right now docs say that the network protocol version for http.client.request.duration metric is always available, which isn't true: https://github.com/dotnet/runtime/blob/55fbb23f8c4e05a17ee27ea19679813f7f5cf285/src/libraries/System.Net.Http/src/System/Net/Http/Metrics/MetricsHandler.cs#L113


Internal previews

📄 File 🔗 Preview link
docs/core/diagnostics/built-in-metrics-system-net.md System.Net metrics

@xakep139
Copy link
Contributor Author

cc @JamesNK @antonfirsov

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! 💯

@lmolkova we cannot populate network.protocol.version when HttpResponseMessage is missing because of an exception. Can we keep the attribute's Requirement Level Recommended in the spec, or shall we change it to Conditionally Required?
https://github.com/open-telemetry/semantic-conventions/blob/main/docs/dotnet/dotnet-http-metrics.md#metric-httpclientconnectionduration

@xakep139
Copy link
Contributor Author

Please merge the PR if you think it's good to go. Thanks 🙇🏻‍♂️

@lmolkova
Copy link
Contributor

lmolkova commented Feb 1, 2024

@lmolkova we cannot populate network.protocol.version when HttpResponseMessage is missing because of an exception. Can we keep the attribute's Requirement Level Recommended in the spec, or shall we change it to Conditionally Required? https://github.com/open-telemetry/semantic-conventions/blob/main/docs/dotnet/dotnet-http-metrics.md#metric-httpclientconnectionduration

We can keep Recommended level - it implies that instrumentation can have good reasons not to populate it. It also means that observability backends should not expect it to be there.

I'll send a tiny PR to the spec to add a description on the level.

@lmolkova
Copy link
Contributor

lmolkova commented Feb 1, 2024

@antonfirsov @xakep139 does this change apply to all client metrics or to http request duration only?

Co-authored-by: Liudmila Molkova <limolkova@microsoft.com>
@xakep139
Copy link
Contributor Author

xakep139 commented Feb 1, 2024

@antonfirsov @xakep139 does this change apply to all client metrics or to http request duration only?

I've found only these usages:

@antonfirsov can you please check whether last two are available if there's no response?

@lmolkova
Copy link
Contributor

lmolkova commented Feb 1, 2024

BTW, any reason to report response version and not a request.Version?

since it's a request duration, it seems reasonable to report request version there, no?

@antonfirsov
Copy link
Member

antonfirsov commented Feb 1, 2024

@antonfirsov @xakep139 does this change apply to all client metrics or to http request duration only?

For the other metrics network.protocol.version is always deducable because they are implemented in SocketsHttpHandler.

any reason to report response version and not a request.Version?

HttpResponseMessage.Version is the actual protocol version used to serve the request, which will be result of the ALPN negotiation and may differ from HttpRequestMessage.Version (or HttpClient.DefaultRequestVersion) set before calling Send. IMO this is the value telemetry users are interested in, request.Version and request.VersionPolicy are only inputs for the version negotiation.

@lmolkova
Copy link
Contributor

lmolkova commented Feb 1, 2024

@antonfirsov thanks for the explanation! Do you think general otel semconv could be more specific and specify that the actual protocol version after negotiation should be used?

@lmolkova
Copy link
Contributor

lmolkova commented Feb 1, 2024

Also, is there any benefit in falling back to request.Version if there was no response? I'm thinking about misconfigured clients and buggy servers/routers that don't respond when protocol is not supported and then you would be interested in request.Version. Super edge-case, but why not use request version as a fallback?

@antonfirsov
Copy link
Member

Do you think general otel semconv could be more specific and specify that the actual protocol version after negotiation should be used

This kind of clarification never hurts, but one should look at version management in other libraries/APIs to phrase it correctly.

Also, is there any benefit in falling back to request.Version if there was no response?

IMO this would be misleading. Imagine the following scenario:

  • A user sets request.Version = 2.0
  • It falls back to 1.1 during negotiation, and an HTTP/1.1 connection starts serving the request
  • Something goes wrong in the middle (eg. invalid response) and an exception is thrown
  • Telemetry would log network.protocol.version=2.0 which was not the actual protocol version used for the failing communication attempt

@xakep139
Copy link
Contributor Author

xakep139 commented Feb 2, 2024

Folks, this PR was intended to fix the documentation and to reflect the current behavior there. My idea was to fix the docs first, and then if we decide - fix the logic/metrics.
I would suggest to continue discussion in the related issue: dotnet/runtime#97395

@antonfirsov antonfirsov merged commit 6f2f2af into main Feb 2, 2024
8 checks passed
@antonfirsov antonfirsov deleted the xakep139-patch-1 branch February 2, 2024 15:03
@antonfirsov
Copy link
Member

Apologies for delaying the merge, I wanted to make sure we are on the same page before going ahead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants